Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Widgets editor] Global inserter open by default #24822

Closed
wants to merge 3 commits into from

Conversation

adamziel
Copy link
Contributor

Description

Before:
Zrzut ekranu 2020-08-26 o 16 09 31

After:
Zrzut ekranu 2020-08-26 o 16 09 24

Related to @mapk explorations in #24561.

This is the first stab at this feature. The code is partially copied from the post editor - to avoid repetition we could extract at least one reusable components here: BlockLibrary sidebar. It's not obvious to me where would it live though - ideas appreciated.

How has this been tested?

  1. Go to the experimental widgets management screen
  2. Confirm the global inserter is open by default

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@adamziel adamziel added [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets labels Aug 26, 2020
@adamziel adamziel requested a review from youknowriad as a code owner August 26, 2020 14:16
@adamziel adamziel self-assigned this Aug 26, 2020
@adamziel adamziel changed the title [Widgets editor] Open global inserter by default [Widgets editor] Global inserter open by default Aug 26, 2020
@github-actions
Copy link

github-actions bot commented Aug 26, 2020

Size Change: +1.7 kB (0%)

Total Size: 1.17 MB

Filename Size Change
build/annotations/index.js 3.67 kB +1 B
build/block-directory/index.js 7.99 kB +1 B
build/block-editor/index.js 126 kB +42 B (0%)
build/block-editor/style-rtl.css 10.8 kB +20 B (0%)
build/block-editor/style.css 10.8 kB +20 B (0%)
build/block-library/editor-rtl.css 8.52 kB +19 B (0%)
build/block-library/editor.css 8.52 kB +19 B (0%)
build/block-library/index.js 136 kB +1.04 kB (0%)
build/block-library/style-rtl.css 7.45 kB +5 B (0%)
build/block-library/style.css 7.46 kB +8 B (0%)
build/blocks/index.js 47.7 kB -2 B (0%)
build/components/index.js 200 kB -9 B (0%)
build/data/index.js 8.55 kB +2 B (0%)
build/edit-post/index.js 304 kB +9 B (0%)
build/edit-site/index.js 17 kB +13 B (0%)
build/edit-widgets/index.js 12.3 kB +454 B (3%)
build/edit-widgets/style-rtl.css 2.5 kB +46 B (1%)
build/edit-widgets/style.css 2.49 kB +45 B (1%)
build/editor/editor-styles-rtl.css 492 B -45 B (9%)
build/editor/editor-styles.css 493 B -46 B (9%)
build/editor/index.js 45.3 kB +31 B (0%)
build/editor/style-rtl.css 3.81 kB +14 B (0%)
build/editor/style.css 3.81 kB +14 B (0%)
build/format-library/index.js 7.71 kB +1 B
build/i18n/index.js 3.57 kB +1 B
build/keyboard-shortcuts/index.js 2.52 kB -1 B
build/media-utils/index.js 5.32 kB -1 B
build/shortcode/index.js 1.7 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.6 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@mtias
Copy link
Member

mtias commented Aug 26, 2020

I think in order to have it open by default we should not allow it to be closed — it should become a region that is always accessible.

@youknowriad
Copy link
Contributor

In terms of a11y, I think we need to bring the "popover" behavior of the "inserter" to edit-widgets screen.
Unfortunately, if we do so, it means we won't be able to make the inserter open by default.

@mtias
Copy link
Member

mtias commented Aug 26, 2020

What I mean is having it open all the time in the same way the current widgets screen has widgets available on the page always visible. And remove the [+] toggle entirely.

@mapk
Copy link
Contributor

mapk commented Aug 26, 2020

What I mean is having it open all the time in the same way the current widgets screen has widgets available on the page always visible. And remove the [+] toggle entirely.

I'm hesitant on this, @mtias. Users will still be able to access the Settings sidebar for blocks too. Keeping the Inserter panel always open takes up a lot of space. Here's a quick mockup to visually convey what I'm talking about. And this is on a fairly wide screen (1278 x 800).

Design notes

  • Added a "Widgets" heading in topbar.
  • Removed the + Inserter

panels

@mtias
Copy link
Member

mtias commented Aug 26, 2020

I don't think that's much of a problem but might be worth trying both. If it's not always open we should not have an "open by default" since it would also absorb focus and we don't want that on loading the page.

@mapk
Copy link
Contributor

mapk commented Aug 26, 2020

I think we're fine showing the Inserter by default. It mimics the behavior of the original widgets screen. I've put this flow together here: #24561 (comment)

It appears to work fine. The Inserter can collapse if needed to save space when opening the Inspector, or simply by clicking the + button in the topbar.

@mtias
Copy link
Member

mtias commented Aug 27, 2020

@mapk there are different implications here since the inserter has popover / modal behaviours. Meaning focus is transferred and contained, which cannot happen on first load of the page as it'd be confusing.

@adamziel
Copy link
Contributor Author

adamziel commented Aug 27, 2020

Could we just not capture focus on the first load in this specific inserter only?

@youknowriad
Copy link
Contributor

I don't think so because as soon as you insert a single widget/block, it will close itself which is confusing IMO.

@mtias
Copy link
Member

mtias commented Aug 28, 2020

As @youknowriad mentions, the interaction model of the inserter as a toggle doesn't lend itself to open by default. The only way I can think of to make it open by default is to remove the toggle behaviour entirely.

@youknowriad
Copy link
Contributor

Potentially it could be a setting "inserter always visible or not" in an "options" modal if there's a big need for it.

@adamziel adamziel requested a review from ellatrix as a code owner August 28, 2020 17:05
@adamziel
Copy link
Contributor Author

adamziel commented Aug 28, 2020

I updated this PR to:

  1. Keep the inserter open by default.
  2. Remove autoFocus on this specific screen only while ensuring the inserter won't close after a block is added.
  3. Make sure that either inserter or sidebar are visible, but not both at the same time.

I didn't update it to use <PopoverWrapper> like it does in post editor, as a result it doesn't close when something else is focused and also it's not wrapped in <FocusManaged> component. Otherwise it works exactly the same. Does using it feel right? @mapk @mtias @youknowriad

If this direction seems right - let's figure out what's missing from this PR and how it could be better in terms of a11y / Gutenberg patterns.
If it doesn't, then what's the other route to try? I'm a bit lost in the discussion at this point :-)

Also, is this comment relevant to the always-open sidebar inserter inspired by the post editor? @youknowriad

In terms of a11y, I think we need to bring the "popover" behavior of the "inserter" to edit-widgets screen.
Unfortunately, if we do so, it means we won't be able to make the inserter open by default.

@mapk
Copy link
Contributor

mapk commented Aug 28, 2020

Thanks, @adamziel !

Make sure that either inserter or sidebar are visible, but not both at the same time.

This interaction should work as it does in the Post Editor now. Opening the Inspector should not close the Inserter unless there's not enough space to show both. I believe there are settings established there that can be adopted here.

Post Editor

sidebars

Does using it feel right?

It started out right to me. I can tab "skip to content" to jump into the topbar. BUT I can't seem to keyboard my way into the Inserter blocks. If I hit Enter it closes the Inserter, and if I click any arrows or tabs, it jumps me to the next topbar icon.

@adamziel
Copy link
Contributor Author

This interaction should work as it does in the Post Editor now. Opening the Inspector should not close the Inserter unless there's not enough space to show both.

That makes sense, thank you! Fortunately it sounds like an easy change.

It started out right to me. I can tab "skip to content" to jump into the topbar. BUT I can't seem to keyboard my way into the Inserter blocks. If I hit Enter it closes the Inserter, and if I click any arrows or tabs, it jumps me to the next topbar icon.

Good notes, thank you! I wonder if that's part of what @youknowriad meant by the popover behavior.

@adamziel
Copy link
Contributor Author

adamziel commented Aug 28, 2020

I talked to @mapk, we compared this interaction to the post editor and the keyboard interaction is actually not trivial to solve with an inserter that doesn't act like a popover - what key should even transfer the focus to the inserter? Enter is for opening/closing. Tab is for navigating the toolbar. Down arrow wouldn't be consistent with other components. Ultimately that's a UX problem to solve, not a bug in the code. I'll defer to @mapk on what should we explore next here.

@mapk
Copy link
Contributor

mapk commented Aug 28, 2020

Because of the complexity, I'd rather we hold off creating a new Inserter interaction that differs from the post editor and move to not include it by default. As your other PR is improving, let's just get the Inserter to mimic the Inserter in the post editor. This way rather than going down a rabbit hole on the Inserter, we can focus on completing the Widget Editor.

@youknowriad
Copy link
Contributor

This is how it used to work on the post editor screen but unfortunately, that's not acceptable as it's an a11y bug. See #24429 and #22858 for more details.

@afercia
Copy link
Contributor

afercia commented Sep 1, 2020

Because of the complexity, I'd rather we hold off creating a new Inserter interaction that differs from the post editor and move to not include it by default. As your other PR is improving, let's just get the Inserter to mimic the Inserter in the post editor.

I'd definitely agree the inserter interaction should be consistent everywhere. Our primary goal should be providing users with a predictable, expected, interaction across the WordPress UI.

Also, keeping the inserter open by default and removing the toggle entirely would force keyboard users to tab through the whole inserter to get to the content each time the use this page. This would make keyboard navigation inefficient. It would also be an assumption that the users flow is always "add" while they might just want to "edit" or "delete" an existing widget (and thus there's no need for the inserter at all).

Potentially it could be a setting "inserter always visible or not" in an "options" modal if there's a big need for it.

Interesting idea. What we're constantly learning with Gutenberg is that users have different needs or even just different preferences. Maybe it's worth reconsidering a bit the WordPress mantra "decisions not options" because a complex user interface needs to cover many different needs. To some extent, options can help. If in the future the need for some users to keep the inserter open will be clear, I wouldn't be opposed to a setting. It could also be a button within the inserter itself, something like "make the inserter sticky" (always open). Such a setting might be useful also in the block editor, for example with very large screens.

@adamziel
Copy link
Contributor Author

It seems like we're pivoting - let's close this PR.

@adamziel adamziel closed this Sep 15, 2020
@youknowriad youknowriad deleted the update/open-inserter-by-default branch September 15, 2020 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants